Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setup HrmpChannelAccepted handler with pallet_xcm for initiating XCM version discovery when openning HRMP channel #4025

Closed
wants to merge 12 commits into from

Conversation

bkontur
Copy link
Contributor

@bkontur bkontur commented Apr 8, 2024

Closes: #922
Implements: #3692 (see: Proposed solution part 2 - initiate VersionDiscoveryQueue when HRMP is accepted).

When we utilize a parachain's pallet_xcm extrinsics or other functionality that employs XcmpQueue as an XcmRouter, there is a possibility of encountering a situation where it is the first message to the desired destination, and we do not know a supported XCM version of the destination. In such cases, pallet_xcm includes a feature called "version discovery," which is initiated. This is a one-time operation. Therefore, this fix aims to initiate "version discovery" as soon as possible when an HRMP channel is created, with best effort.

In other words, when parachains establish an HRMP channel, we immediately initiate XCM version discovery and don't wait for the very first user request.

This PR:

@bkontur bkontur added the T6-XCM This PR/Issue is related to XCM. label Apr 8, 2024
@bkontur bkontur requested a review from a team as a code owner April 8, 2024 11:13
impl<T: Config> HandleHrmpChannelAccepted for Pallet<T> {
fn handle(recipient: u32) -> XcmResult {
// how would the actual runtime see the `recipient` as `Parachain(recipient)`
let dest = match T::UniversalLocation::get().global_consensus() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious - I though that UniversalLocation is assumed to be started with a GlobalConsensus. Then why would we need that match here? A defensive programming or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious - I though that UniversalLocation is assumed to be started with a GlobalConsensus. Then why would we need that match here? A defensive programming or something else?

Yes, good point, exactly - defensive and generic.

Originally, I wanted to use just let dest = ParentThen(Parachain(recipient).into()).into();, but yeah there is a Parent which is also not that generic.

Yes, there is that thing with GlobalConsensus you mentioned, which I guess it's kind of unwritten rule or assumption that runtime's UniversalLocation starts with GlobalConsensus - I don't remember exactly but without GlobalConsensus something fails in XcmExecutor (maybe just ExportMessage or something with WithComputedOrigin...)

The Hrmp notifications have just paraId: u32 and we need to convert it to xcm::Location, so I was thinking this way:
*Parachain(recipient)*.relative_to(&T::UniversalLocation::get());.

        #[test]
	fn test() {
		// E.g. we are on AssetHub
		frame_support::parameter_types! {
			pub UniversalLocation: InteriorLocation = [GlobalConsensus(Rococo), Parachain(1000)].into();
		}

		// We opened channel with 1003
		let recipient = 1003;

		// convert 1003 from the point of view of `UniversalLocation`
		let dest = match UniversalLocation::get().global_consensus() {
			Ok(global_consensus) =>
				Junctions::from([GlobalConsensus(global_consensus), Parachain(recipient)]),
			Err(_) => Junctions::from([Parachain(recipient)]),
		}.relative_to(&UniversalLocation::get());

		// expected result, how AssetHub sees sibling parachain
		assert_eq!(dest, Location { parents: 1, interior: Junctions::X1([Parachain(1003)].into()) });
	}

And when I was checking other runtimes, I saw that we have some minimal template without GlobalConsensus: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/runtime/src/configs/xcm_config.rs#L29, so thats why I ended up with that match.

@svyatonik Do you have any other idea how to handle a conversion from paraId: u32 to the Location with T::UniversalLocation? I was thinking about some adapter with which would wrap Convert<u32, Location>, Self::get_version_for with Self::note_unknown_version, but it could just complicate things and not sure if it is worth.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though that UniversalLocation is assumed to be started with a GlobalConsensus.

Yes, this is ideally where we should get, but not all parachains have done this yet..

Everything except bridging works fine without it too, so there is also no big incentive to do it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And when I was checking other runtimes, I saw that we have some minimal template without GlobalConsensus: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/runtime/src/configs/xcm_config.rs#L29

We should sanitize whole repo and make sure all our runtimes test, mock, etc have the GlobalConsensus too so we create a consistent best-practice.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Originally, I wanted to use just let dest = ParentThen(Parachain(recipient).into()).into();, but yeah there is a Parent which is also not that generic.

But OTOH we know that this notification comes from the relay chain (because it notifies us about new HRMP channels), right? So maybe Parent would be enough here :) Not sure if it'll be changed

And when I was checking other runtimes, I saw that we have some minimal template without GlobalConsensus: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/runtime/src/configs/xcm_config.rs#L29, so thats why I ended up with that match.

Maybe we should fix that in this case. E.g. some bridging adapters (that's where I got this assumption - e.g. UnpaidRemoteExporter) are assuming that UniversalLocation starts with GlobalConsensus. But that's not related to this PR ofc.

Thank you for the explanation - I've just been curios about that && I wasn't planning to request changes or smth like that :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And when I was checking other runtimes, I saw that we have some minimal template without GlobalConsensus: https://github.com/paritytech/polkadot-sdk/blob/master/templates/parachain/runtime/src/configs/xcm_config.rs#L29

We should sanitize whole repo and make sure all our runtimes test, mock, etc have the GlobalConsensus too so we create a consistent best-practice.

Good point, attempt to sanitize (where possible): 9c5a918

@bkontur bkontur mentioned this pull request Apr 12, 2024
2 tasks
@acatangiu
Copy link
Contributor

@bkontur is this good to merge?

@bkontur
Copy link
Contributor Author

bkontur commented Apr 12, 2024

@bkontur is this good to merge?

not yet, I want to check comments and add also closing part, where we clean storage for unneded stuff

@bkontur bkontur requested a review from athei as a code owner April 15, 2024 08:40
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5926429

@bkontur bkontur removed the request for review from athei April 15, 2024 12:26
@bkontur
Copy link
Contributor Author

bkontur commented Apr 15, 2024

  • investigating one more test if this is enough and aligned with process_hrmp_open_channel_requests / initializer_on_new_session

@bkontur
Copy link
Contributor Author

bkontur commented Apr 16, 2024

Changing to the draft, because HrmpNotificationAccepted won't work and need to find another solution: #922 (comment)

Opened another PR: #4156 which contains still relevant changes but with reverted HrmpNotificationAccepted handler stuff.

github-merge-queue bot pushed a commit that referenced this pull request Apr 22, 2024
… and improvements (#4238)

This PR:
- sanitizes all `UniversalLocation`s with `GlobalConsensus` (when
possible) - addressing
[comment](#4025 (comment))
- adds `DefaultConfig` for `pallet-xcm-benchmarks` for `system`
@bkontur bkontur closed this Nov 14, 2024
@bkontur bkontur deleted the bko-hrmp-and-version-discovery branch November 14, 2024 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T6-XCM This PR/Issue is related to XCM.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

xcm version auto discovery improvements
6 participants